Skip to content

[fix][proxy] Fix incorrect client error when calling get topic metadata #24181

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Apr 17, 2025
Merged

[fix][proxy] Fix incorrect client error when calling get topic metadata #24181

merged 3 commits into from
Apr 17, 2025
+86 −2

Conversation

poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented Apr 15, 2025 •
edited
Loading

Motivation

Proxy overwrites multiple error codes to LookupError, which may cause issues that many expressions, such as ex instance of TopicDoesNotExistException get a false result even if the broker responds a TopicDoesNotExistException

The code lines that use the expression ex instance of TopicDoesNotExistException

Modifications

  • Correct the error code

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: x

@poorbarcode poorbarcode self-assigned this Apr 15, 2025
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 15, 2025
@poorbarcode poorbarcode added type/bug The PR fixed a bug or issue reported a bug area/proxy and removed doc-not-needed Your PR changes do not impact docs labels Apr 15, 2025
@poorbarcode poorbarcode added this to the 4.1.0 milestone Apr 15, 2025
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 15, 2025
@poorbarcode
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@nodece
Copy link
Member

nodece commented Apr 16, 2025

I suggest you check the org.apache.pulsar.client.api.PulsarClientException#wrap and org.apache.pulsar.client.api.PulsarClientException#unwrap calls to get the correct error.

@poorbarcode
Copy link
Contributor Author

@nodece @liangyepianzhou

I suggest you check the org.apache.pulsar.client.api.PulsarClientException#wrap and org.apache.pulsar.client.api.PulsarClientException#unwrap calls to get the correct error.

Good pointer, changed the code

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 15.00000% with 34 lines in your changes missing coverage. Please review.

Project coverage is 74.23%. Comparing base (bbc6224) to head (e6cf687).
Report is 1026 commits behind head on master.

Files with missing lines Patch % Lines
.../java/org/apache/pulsar/client/impl/ClientCnx.java 8.10% 22 Missing and 12 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #24181      +/-   ##
============================================
+ Coverage     73.57%   74.23%   +0.65%     
+ Complexity    32624    32508     -116     
============================================
  Files          1877     1865      -12     
  Lines        139502   144628    +5126     
  Branches      15299    16521    +1222     
============================================
+ Hits         102638   107358    +4720     
+ Misses        28908    28777     -131     
- Partials       7956     8493     +537
Flag Coverage Δ
inttests 26.72% <0.00%> (+2.13%) ⬆️
systests 23.19% <0.00%> (-1.13%) ⬇️
unittests 73.72% <15.00%> (+0.88%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...apache/pulsar/proxy/server/LookupProxyHandler.java 60.41% <100.00%> (-0.18%) ⬇️
.../java/org/apache/pulsar/client/impl/ClientCnx.java 69.62% <8.10%> (-2.16%) ⬇️

... and 1072 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@poorbarcode poorbarcode force-pushed the fix/proxy_lookup_error branch from bfb457f to f787558 Compare April 17, 2025 14:39
nodece
nodece approved these changes Apr 17, 2025
View reviewed changes
Copy link
Member

@nodece nodece left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good.

@nodece nodece requested a review from Copilot April 17, 2025 15:08
Copilot
Copilot AI reviewed Apr 17, 2025
View reviewed changes
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@nodece nodece requested a review from Copilot April 17, 2025 15:09
Copilot
Copilot AI reviewed Apr 17, 2025
View reviewed changes
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java:1385

  • [nitpick] The method name 'revertClientExToErrorCode' could be more descriptive. Consider renaming it to 'mapClientExceptionToServerError' for clarity.
public static ServerError revertClientExToErrorCode(PulsarClientException ex) {

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/LookupProxyHandler.java:250

  • [nitpick] The variable name 'pce' could be more descriptive; consider renaming it to 'clientException' to improve readability.
PulsarClientException pce = PulsarClientException.unwrap(t);

@lhotari lhotari changed the title [fix][proxy]Fix incorrect client error when calling get topic metadata [fix][proxy] Fix incorrect client error when calling get topic metadata Apr 17, 2025
@lhotari lhotari merged commit 39f3d9b into apache:master Apr 17, 2025
53 of 54 checks passed
lhotari pushed a commit that referenced this pull request Apr 17, 2025
@poorbarcode @lhotari
[fix][proxy] Fix incorrect client error when calling get topic metada…
2a52ad2 
…ta (#24181)

(cherry picked from commit 39f3d9b)
lhotari pushed a commit that referenced this pull request Apr 17, 2025
@poorbarcode @lhotari
[fix][proxy] Fix incorrect client error when calling get topic metada…
81fffc4 
…ta (#24181)

(cherry picked from commit 39f3d9b)
lhotari pushed a commit that referenced this pull request Apr 17, 2025
@poorbarcode @lhotari
[fix][proxy] Fix incorrect client error when calling get topic metada…
5b349db 
…ta (#24181)

(cherry picked from commit 39f3d9b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@lhotari lhotari lhotari approved these changes

@Technoboy- Technoboy- Technoboy- approved these changes

@nodece nodece nodece approved these changes

@liangyepianzhou liangyepianzhou Awaiting requested review from liangyepianzhou

Assignees

@poorbarcode poorbarcode

Labels
area/proxy cherry-picked/branch-3.0 cherry-picked/branch-3.3 cherry-picked/branch-4.0 doc-not-needed Your PR changes do not impact docs ready-to-test release/3.0.12 release/3.3.7 release/4.0.5 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
4.1.0
Development

Successfully merging this pull request may close these issues.
5 participants
@poorbarcode @nodece @codecov-commenter @lhotari @Technoboy-